Skip to content

add requiredFirst option #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 15, 2020
Merged

Conversation

nhannah
Copy link
Contributor

@nhannah nhannah commented May 13, 2020

This is a solution for allowing preference to be given to non-optionals. This option will basically split the type in 2 for sorting with non-optionals first in the list. Required types first is a standard quite often it seems so with sorting it allows for the best of both worlds.

@nhannah nhannah changed the title add requiredFirst as string prefix solution add requiredFirst option May 13, 2020
@infctr
Copy link
Owner

infctr commented May 13, 2020

Thank you for the PR!

Unfortunately there seem to be no tests for the "requiredFirst: false" option. Also please add some test cases for the autofix

@nhannah
Copy link
Contributor Author

nhannah commented May 14, 2020

Thank you for the PR!

Unfortunately there seem to be no tests for the "requiredFirst: false" option. Also please add some test cases for the autofix

Hey, so I have made the tests but I also found an issue I am not 100% on in the current code:

type Type1<TKey extends string> = Partial<{
  // %foo
  foo: boolean;
  /* %baz */ baz: boolean;

  /**
   * %bar
   */
  bar: boolean;
  /* %bam */ bam: boolean;
}> & {/* %foo */
  foo: boolean;

// %baz
  baz: boolean;
  /**
   * %bar
   */
  bar?: boolean;
} & {
    [K in keyof TKey]: boolean;
  };

If you add bam to the bottom of this type you will get the error Fix objects must not be overlapped in a report. I'll push the new tests and the new option should function as the old plugin does with the addition, but I haven't fixed that potential error in either the new or old code. It seems to be tied to the way comments are autofixed; but I did not dig into it more.

@infctr
Copy link
Owner

infctr commented May 14, 2020

@nhannah Thanks for updating tests! I'll investigate fixer failure later, seems like an edge case.

The PR looks good to me, I'll merge and publish it shortly. I've been working on a complete Typescript rewrite in the meantime which is almost finished, so I'll have to incorporate your changes smh 😄

@infctr infctr merged commit 09b790c into infctr:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants